feat: add tool usage recording for blocking responses interceptor #122
Conversation
pawbana
force-pushed
the
pb/responses-record-tool-usage
branch
2 times, most recently
from
b0290a7 to
ee80a4b
Compare
intercept/responses/base_test.go
Outdated
| expected: nil, | ||
| }, | ||
| { | ||
| name: "nested_object", |
responses_integration_test.go
Outdated
| recordedTools[0].InterceptionID = tc.expectToolRecorded.InterceptionID // ignore interception id | ||
| recordedTools[0].CreatedAt = tc.expectToolRecorded.CreatedAt // ignore time |
There was a problem hiding this comment.
alternatively you can clear both fields if ignored
intercept/responses/base.go
Outdated
| func (i *responsesInterceptionBase) parseJSONArgs(raw string) recorder.ToolArgs { | ||
| if trimmed := strings.TrimSpace(raw); trimmed != "" { | ||
| var args recorder.ToolArgs | ||
| if err := json.Unmarshal([]byte(trimmed), &args); err == nil { |
There was a problem hiding this comment.
should we do something if JSON is borked?
There was a problem hiding this comment.
Agreed; errors should be handled.
There was a problem hiding this comment.
Thanks. Logic was wrong here. Updated it to match what is done in chat completions.
| ID: "resp_789", | ||
| Output: []oairesponses.ResponseOutputItemUnion{ | ||
| { | ||
| Type: "function_call", |
There was a problem hiding this comment.
hm... do we need another test for function_call with broken args?
intercept/responses/base.go
Outdated
| } | ||
| } | ||
|
|
||
| func (i *responsesInterceptionBase) parseJSONArgs(raw string) recorder.ToolArgs { |
There was a problem hiding this comment.
This method isn't well named. It should indicate that it's just for tools.
intercept/responses/base.go
Outdated
| func (i *responsesInterceptionBase) parseJSONArgs(raw string) recorder.ToolArgs { | ||
| if trimmed := strings.TrimSpace(raw); trimmed != "" { | ||
| var args recorder.ToolArgs | ||
| if err := json.Unmarshal([]byte(trimmed), &args); err == nil { |
There was a problem hiding this comment.
Agreed; errors should be handled.
intercept/responses/base.go
Outdated
| // recodring other function types to be considered: https://github.com/coder/aibridge/issues/121 | ||
| switch item.Type { | ||
| case "function_call": | ||
| args = i.parseJSONArgs(item.Arguments) |
There was a problem hiding this comment.
Why is this necessary to do? Why can't we pass the string value like we do for the others?
By the time it's stored in the db it's a string anyway.
There was a problem hiding this comment.
Just wanted to keep it consistent with chat completions.
You are right this seems redundant with how we store things in DB but then recorder API should change (ToolUsageRecord should have Args as string instead of any)?
responses_integration_test.go
Outdated
| recordedTools := mockRecorder.RecordedToolUsages() | ||
| if tc.expectToolRecorded != nil { | ||
| require.Len(t, recordedTools, 1) | ||
| recordedTools[0].InterceptionID = tc.expectToolRecorded.InterceptionID // ignore interception id |
There was a problem hiding this comment.
Please provide a reason why.
pawbana
force-pushed
the
pb/responses-record-tool-usage
branch
from
8ecf6e4 to
7bdf03a
Compare
pawbana
force-pushed
the
pb/responses-record-tool-usage
branch
from
7bdf03a to
11d3c46
Compare
3 participants
No description provided.